2018 Fantasy Football API Update #615
Conversation
…orks in the event of 401 error
|
This also addresses my issue #278. |
|
Made the changes & tested it against my previously failing 2018 league. Updates worked for all league years (2017 and prior, 2018, and 2019 and newer). Thanks for the suggestion on where to add this check into the code, and hope the changes make sense. |
|
I saw the original tests failed because of the urllib3 version issue. I reran the tests for this PR after you updated/pinned that version earlier today and got the same errors on the test cases that came thru on the next github actions workflow. From my review so far, it looks like because of where I implemented the "check_league_endpoint" function within the class init it is being missed by the test cases. Taking another look tonight and I'm reconsidering the implementation I initially had with a separate function. This might be more appropriate to place this logic within your existing checkRequestStatus function, which is already being run in both get/league_get. |
|
OK, hopefully fourth time is a charm!
def test_private_league(self):
with self.assertRaises(Exception):
League(368876, 2018)It looks like this test is expecting an exception to be raised, but manually running additional tests with that LeagueID works fine w/o cookies (seems like I don't need cookies for my own league, 747376, either). Was there something else this test was checking for? |
espn_api/requests/espn_requests.py
Outdated
| @@ -17,19 +17,49 @@ class ESPNUnknownError(Exception): | |||
| pass | |||
|
|
|||
|
|
|||
| def checkRequestStatus(status: int, cookies=None, league_id=None) -> None: | |||
| def checkRequestStatus(status: int, cookies=None, league_id=None, league_endpoint=None, year=None, extend="", params=None, headers=None) -> tuple: | |||
There was a problem hiding this comment.
To make this function a little simpler, I think moving this inside the request class so then you won't have to pass all these fields to it. You then can access all of them through this.
There was a problem hiding this comment.
Yes I was actually considering that last night, but wasn't sure if you had a reason it was defined outside the class. Moving it inside would definitely simplify access to those params for sure.
|
Excellent -- I was experimenting with both your League(368876, 2018) and my League(747376, 2018) this morning and it seems like both of those are accessible at the 'leagueHistory' endpoint. So in your test_private_league, it will now not raise an exception with this code, causing the test to fail. At least I think that is what is happening, are you able to validate that assumption? |
Wow is it really? Thats an old private league of mine and wouldn't think it would be accessible. I wonder if we update to an older year it would throw? |
Yeah my league is private too and I can access yours and mine without cookies, for all of the older years (mine goes back to 2013). However, I know this original issue was sporadic on some leagues, so I'm hesitant to say that a simple change to make the endpoint for 2018 always use the older URL would work for everyone. |
Nice, well thats a little added feature. Definitely a weird bug on ESPN's side but glad you found a fix for it! |
|
Ok I moved the checkRequestStatus function into the class. Everything seems to be working on my end & ran all the tests, and it is still failing just the one I referenced above for your 2018 private league. How do you want me to handle that one test case failure? I'm not sure how to test that private league feature now since it doesn't appear to be a restriction w/ the data I have. |
I think we can use that test to get coverage on the switching endpoint logic. It can be updated to not throw error on private league. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #615 +/- ##
==========================================
- Coverage 83.21% 83.13% -0.09%
==========================================
Files 61 61
Lines 2300 2306 +6
==========================================
+ Hits 1914 1917 +3
- Misses 386 389 +3 ☔ View full report in Codecov by Sentry. |
I modified the test_private_league to validate the LEAGUE_ENDPOINT URL it switches to (i.e. it falls back to the I re-read your comment and now realize you had said the test could check that it does not throw an error for a private league. I can modify back to that if you'd prefer or just add that back as an additional test case if you see value in the one I modified here. Interesting side-note, it appears the |
This looks good! Interesting, thats kind of nice! |
Took another look at the code and made a second attempt to fix #514. The issue is caused by lines 48-51 in espn_requests.py, where the endpoint used for 2018 league in my case should be the older endpoint. When that endpoint is used, the data returned has a 0 index and needs to be handled appropriately. Updates made in base_league.py to handle the 0 index JSON data. Additionally, created a new function in epsn_requests.py to validate if endpoint causes a 401 and if so, try the other endpoint, raise an error if both fail.